oidcauth: reduce provider re-initialization in TestOIDCAuthorization_UserinfoPaths#169790
Open
sanchit-CRL wants to merge 1 commit intocockroachdb:masterfrom
Open
oidcauth: reduce provider re-initialization in TestOIDCAuthorization_UserinfoPaths#169790sanchit-CRL wants to merge 1 commit intocockroachdb:masterfrom
sanchit-CRL wants to merge 1 commit intocockroachdb:masterfrom
Conversation
Contributor
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Member
|
This change is |
cockroach-teamcity
added
the
X-perf-gain
sanchit-CRL
force-pushed
the
worktree-fix-oidc-userinfo-test
branch
from
7183619 to
4eef28a
Compare
…UserinfoPaths TestOIDCAuthorization_UserinfoPaths was flaking under race detection because each of 7 test cases created a new httptest.Server and overrode OIDCProviderURL + OIDCRedirectURL cluster settings. Each override triggered reloadConfig() → NewOIDCManager() → HTTP discovery request, producing ~14 provider discovery roundtrips per test run. Under race detection on slow CI machines (s390x), this compounded to 20s+ latency, exceeding the HTTP client timeout. Restructure the test to share a single mock OIDC provider across the 5 cases that have identical discovery documents (all with a userinfo_endpoint), swapping only the /userinfo response between cases via a mutex-protected variable. Set all cluster settings once before the loop with OIDCEnabled last, so only one provider discovery call occurs during setup. The 2 special cases (absent endpoint, network error) that need different discovery documents run separately with their own mock servers. This reduces provider discovery HTTP calls from ~14 to 3 and eliminates all settings changes during the main test loop, removing the mutex contention between reloadConfig callbacks and the callback handler that caused the timeouts. Fixes: cockroachdb#152684 Release note: None Epic: none Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
sanchit-CRL
force-pushed
the
worktree-fix-oidc-userinfo-test
branch
from
4eef28a to
4d33f09
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TestOIDCAuthorization_UserinfoPaths was flaking under race detection because each of 7 test cases created a new httptest.Server and overrode OIDCProviderURL + OIDCRedirectURL cluster settings. Each override triggered reloadConfig() → NewOIDCManager() → HTTP discovery request, producing ~14 provider discovery roundtrips per test run. Under race detection on slow CI machines (s390x), this compounded to 20s+ latency, exceeding the HTTP client timeout.
Restructure the test to share a single mock OIDC provider across the 5 cases that have identical discovery documents (all with a userinfo_endpoint), swapping only the /userinfo response between cases via a mutex-protected variable. Set all cluster settings once before the loop with OIDCEnabled last, so only one provider discovery call occurs during setup. The 2 special cases (absent endpoint, network error) that need different discovery documents run separately with their own mock servers.
This reduces provider discovery HTTP calls from ~14 to 3 and eliminates all settings changes during the main test loop, removing the mutex contention between reloadConfig callbacks and the callback handler that caused the timeouts.
Fixes: #152684
Release note: None
Epic: none